New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Re-implement parameterizer #28
Conversation
0de7e89
to
fa26ea8
Compare
Fixes a bug related to the resolution of param proposals with challenges not processed until after the processBy date. Previously, tokens in the challenge would be burned. Now the challenge is resolved, but the param is not updated no matter what if the processBy < now.
fa26ea8
to
1807496
Compare
1807496
to
90236bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me @skmgoldin . Will write some tests for this bad boy soon. If you'd feel more comfortable, we can do a code review together, but I've looked over this and I'm comfortable with pulling it for you!
contracts/Parameterizer.sol
Outdated
function canBeSet(bytes32 _propID) constant public returns (bool) { | ||
ParamProposal memory prop = proposalMap[_propID]; | ||
|
||
return (propExists(_propID) && now > prop.appExpiry && now < prop.processBy && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's test this, but I think propExists(_propID)
is extra because now < prop.processBy
will effectively take care of this check.
set(prop.name, prop.value); | ||
} else if (challengeCanBeResolved(_propID)) { | ||
resolveChallenge(_propID); | ||
} else if (now > prop.processBy) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Safety checks and require(token.transfer(prop.owner, reward));
ParamProposal memory prop = proposalMap[_propID]; | ||
uint deposit = get("pMinDeposit"); | ||
|
||
require(propExists(_propID) && prop.challengeID == 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this require be encapsulated as a modifier?
contracts/Parameterizer.sol
Outdated
} | ||
|
||
// set flag on challenge being processed | ||
challengeMap[prop.challengeID].resolved = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a design pattern, should the state transition be done before the transfer (even though re-entrancy may not possible here).
contracts/Parameterizer.sol
Outdated
@param _challengeID The challengeID to determine a reward for | ||
*/ | ||
function determineReward(uint _challengeID) public constant returns (uint) { | ||
require(!challengeMap[_challengeID].resolved && voting.pollEnded(_challengeID)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary check?
Thanks for the code review, will update. |
Parameterization is considered an unsolved problem in token-curated registries; there is no formal purity in any of the proposed solutions, and there are known attacks. The formal weakness in TCR parameterizers is that they may be put into “stuck states”, EG, the COMMIT_STAGE_LEN parameter could be set to be very long, effectively breaking challenges. The AdChain parameterizer is an attempt at a practically safe if formally-impure cryptosystem which retains the registry's property of being completely decentralized. In this design, financially-motivated token holders are incentivized to be diligent and not allow “breaking” proposals to be accepted. There is no safety net after that, however.
Mechanically, the AdChain parameterizer implements much of the same logic as the registry itself. The main differences are that instead of
Listings
we use ParamProposals, and deposits are simply returned if a proposal is accepted without a challenge. This is to say, set parameters are not “challenged” the way listings are. Instead, a new proposal is made and either accepted without challenge or, if challenged, perhaps accepted or perhaps rejected. When a new proposal is made, AdToken is at stake and challenges are thereby incentivized.The AdChain parameterizer has its own parameters separate from those of the registry itself. The parameters prepended with
p
such aspMinDeposit
are parameters of the parameterizer itself. This is a safety feature: because re-parameterizations are potentially dangerous operations, logically all of the working parameters should be set significantly higher than in the registry itself to mitigate spam and un-serious actors from putting the system at risk.ParamProposals
have a processBy date hardcoded to seven days plus the duration of the apply, reveal and commit stages. This is another safety feature which basically requires thatParamProposals
be processed within some reasonable period of time after which they cannot beset
no matter what (though challenges on them can be resolved for the purposes of disbursing tokens). The reason for this is to prevent an attacker from securing an acceptance vote on a re-parameterization which makes sense at timea
, but which the attacker sits on until timeb
to deploy when that re-parameterization may not make sense. This is implemented here.Additional safety features include disallowing duplicate re-parameterizations, meaning that if param
x
is set toa
, a proposal to setx
toa
will be automatically rejected. Furthermore, if a proposal exists to setx
tob
, follow-on proposals to do the same will be automatically rejected until the first is processed. This is just to reduce spamming, as well as cognitive overhead for voters (“but I thought we rejected that proposal?”).Not absolutely everything discussed in this PR is actually fully implemented yet, but I would estimate 80% of it is. The test suite is not complete either and it has not been user tested, so it probably has bugs. Furthermore, there is logic related to challenges which is duplicated between Registry.sol and Parameterizer.sol. All of these will be addressed in future pull requests, but I want to merge this to master so we can ship it to the application team by middle of the week. The existing parameterizer is unused by them, so it won’t break anything.